fix(tui): 分离执行活动并恢复输出区滚动#117
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
/review |
| Detail: detail, | ||
| IsError: isError, | ||
| }) | ||
| if len(a.activities) > maxActivityEntries { |
There was a problem hiding this comment.
Inefficient circular buffer implementation causes unnecessary allocations
This pattern allocates a new backing array and copies 64 elements on every append once the buffer is full. This creates GC pressure during rapid event streams and is inefficient.
Suggested fix: Use simple slice reslicing:
if len(a.activities) > maxActivityEntries {
a.activities = a.activities[len(a.activities)-maxActivityEntries:]
}Or for a more robust solution, consider implementing a proper ring buffer with fixed-size array and head/tail indices.
| return a, tea.Batch(cmds...) | ||
| case RuntimeClosedMsg: | ||
| a.state.IsAgentRunning = false | ||
| if strings.TrimSpace(a.state.StatusText) == "" { |
There was a problem hiding this comment.
Removed rebuildTranscript() may cause stale display
The original code called rebuildTranscript() after runtime closure, but this was removed. If there are pending changes to activeMessages not yet reflected in the viewport, the user might see stale content.
Suggested fix: Verify that all message updates set transcriptDirty = true before runtime closure, or restore a conditional rebuild here.
| bodyY := contentY + headerHeight | ||
|
|
||
| streamX := contentX | ||
| streamY := bodyY |
There was a problem hiding this comment.
Layout recomputed on every mouse event
The transcriptBounds() method calls computeLayout() and renderHeader() on every mouse wheel event. During rapid scrolling, this adds unnecessary CPU overhead.
Suggested fix: Cache layout bounds after resize events and reuse them:
// In App struct
type App struct {
// ...
cachedTranscriptBounds struct {
x, y, width, height int
valid bool
}
}
// Invalidate on resize
func (a *App) resizeComponents() {
a.cachedTranscriptBounds.valid = false
// existing logic...
}| @@ -47,18 +49,19 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { | |||
| a.resizeComponents() | |||
| return a, tea.Batch(cmds...) | |||
There was a problem hiding this comment.
Return value behavior undocumented
The handleRuntimeEvent function now returns a bool indicating whether the transcript needs to be rebuilt. This is a significant behavioral change that should be documented.
Suggested fix: Add function comment:
// handleRuntimeEvent processes runtime events and returns true if the
// transcript needs to be rebuilt (i.e., when new messages are appended
// to activeMessages).
func (a *App) handleRuntimeEvent(event runtime.Event) (transcriptDirty bool) {| focusLabelActivity = "Activity" | ||
| focusLabelComposer = "Composer" | ||
|
|
||
| activityPreviewEntries = 3 |
There was a problem hiding this comment.
Missing documentation for magic numbers
These constants lack comments explaining their purpose and rationale.
Suggested fix:
// activityPreviewEntries is the number of recent activity entries shown in the preview panel
activityPreviewEntries = 3
// maxActivityEntries is the maximum number of activity entries retained in memory to prevent unbounded growth
maxActivityEntries = 64| } | ||
| } | ||
|
|
||
| func (a App) activityPreviewHeight() int { |
There was a problem hiding this comment.
Magic number for fixed height
The function returns hardcoded 6 without explanation. This could cause layout issues if activityPreviewEntries changes.
Suggested fix: Add comment explaining the calculation:
// activityPreviewHeight returns the height of the activity preview panel.
// Returns 6 when activities exist (2 for header/border + 3 entry lines + 1 padding), 0 otherwise.
func (a App) activityPreviewHeight() int {| @@ -230,8 +227,7 @@ func (a App) updateInputPanel(msg tea.Msg, typed tea.KeyMsg, cmds []tea.Cmd) (te | |||
| if err := a.refreshModelPicker(); err != nil { | |||
There was a problem hiding this comment.
Tool result message created without Name field
When appending tool results to activeMessages, the message is created without preserving the tool name. The payload.Name is only logged in the activity.
Suggested fix: Consider whether the Name field should be preserved for transcript rendering consistency. Review if renderMessageBlock expects tool name information.
Code Review SummaryOverall, this is a well-structured PR that successfully separates activity execution from the transcript and restores scrolling. The test coverage is comprehensive. The implementation follows existing patterns and improves UX. Key strengths: Clean separation of concerns with Priority fixes: Address the inefficient circular buffer allocation (update.go:539), add string length limits to activity entries (security), and clarify the unreachable Great work on the activity tracking feature! 👍 |
| a.activeMessages = append(a.activeMessages, provider.Message{Role: role, Content: content}) | ||
| } | ||
|
|
||
| func (a *App) appendActivity(kind string, title string, detail string, isError bool) { |
There was a problem hiding this comment.
Missing string length limits and function documentation
The title and detail parameters are stored without length validation. While maxActivityEntries limits entry count, each entry's strings are unbounded. A malicious or misbehaving provider could send extremely long payloads causing excessive memory consumption.
Suggested fixes:
- Add length limits:
const maxTitleLen = 256
const maxDetailLen = 1024
title = strings.TrimSpace(title)
detail = strings.TrimSpace(detail)
if len(title) > maxTitleLen {
title = title[:maxTitleLen-3] + "..."
}
if len(detail) > maxDetailLen {
detail = detail[:maxDetailLen-3] + "..."
}- Add function documentation:
// appendActivity adds an execution event to the activity log.
// kind categorizes the event (tool, command, provider, etc.),
// title is the main display text, detail provides additional context,
// and isError marks error states. Empty entries are ignored.
func (a *App) appendActivity(kind string, title string, detail string, isError bool) {| @@ -418,11 +421,58 @@ func (a App) focusLabel() string { | |||
| return focusLabelSessions | |||
There was a problem hiding this comment.
Unreachable focus state for panelActivity
The focusLabel() method handles panelActivity, but focusNext() and focusPrev() in update.go only cycle through [panelSessions, panelTranscript, panelInput]. The Activity panel cannot be focused via keyboard navigation.
Suggested fix: Either add panelActivity to the focus order array, or remove this case from focusLabel() if it's intentionally unfocusable.
Closes #99
说明
验证